Skip to content

taking :skip in nested collection into account#54

Open
thedarkside wants to merge 1 commit into
nickcharlton:mainfrom
thedarkside:master
Open

taking :skip in nested collection into account#54
thedarkside wants to merge 1 commit into
nickcharlton:mainfrom
thedarkside:master

Conversation

@thedarkside
Copy link
Copy Markdown
Contributor

This solves #12 by taking the :skip option into account while rendering nested collections.

Currently i don't have time to implement tests. Maybe someone wants to volunteer? :)

Works as expected in our current implementation using v1.3.0

@jubilee2
Copy link
Copy Markdown
Contributor

jubilee2 commented Aug 5, 2022

looks good for me

module Page
class NestedCollection < Page::Collection
def attribute_names
options.fetch(:collection_attributes, dashboard.collection_attributes)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the following? That way you only call dashboard.collection_attributes lazily, when there is no :collection_attributes available:

Suggested change
options.fetch(:collection_attributes, dashboard.collection_attributes)
options.fetch(:collection_attributes) { dashboard.collection_attributes }

@nickcharlton
Copy link
Copy Markdown
Owner

Is there anything we could do for testing here?

@jubilee2
Copy link
Copy Markdown
Contributor

jubilee2 commented Aug 8, 2023

@thedarkside do you want put this in field of hasMany module on main administrate project?

      def associated_collection(order = self.order)
        Administrate::Page::NestedCollection.new(associated_dashboard, order: order, collection_attributes: options.fetch(:collection_attributes))
      end

and in Page::Collection

def attribute_names
        options.fetch(:collection_attributes) || dashboard.collection_attributes
end

@goosys
Copy link
Copy Markdown

goosys commented May 2, 2024

Looks good for me.
Do you have plans to merge this change? Is there anything I can assist with?

@nickcharlton
Copy link
Copy Markdown
Owner

We need to add some test coverage around the change, that's the main thing, if you'd like to take a look, @goosys?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants